Fix autosend scheduling #6121

seadowg · 2024-05-02T09:53:13Z

This also:

Fixes problems introduced recently where form level auto send was no longer working
Makes sure that pending auto send jobs will run after an app upgrade (even if there is a change in the underlying code)
Will not run "Cellular only" auto send when connected to a metered wifi (like a hotspot), but will retry later (with an exponential back off)

It's probably a good idea to wait on merging this until it's marked "behaviour verified" as we've seen a lot of churn on auto send recently.

Why is this the best possible solution? Were any other approaches considered?

I would have loved to have a more elegant solution for "cellular only", but couldn't find one that would work within the WorkManager world. I think the compromise of having other settings work optimally within Android's current background job setup is correct though.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This should mean that auto send now behaves more like how we'd "expect". One caveat to highlight is that metered wifi (like a hotspot) will not work with "wifi only" or "cellular only". The work around for this (other than using "wifi or cellular") is to change the wifi to "unmetered" in Android's settings.

Before submitting this PR, please make sure you have:

added or modified tests for any new or changed behavior
run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
added a comment above any new strings describing it for translators
verified that any code or assets from external sources are properly credited in comments and/or in the about file.
verified that any new UI elements use theme colors. UI Components Style guidelines

seadowg · 2024-05-03T11:11:48Z

@WKobus could you take another look at this when you get a chance? The biggest change since you last looked is that "cellular only" shouldn't run when connected to a metered wifi (like a hotspot), but should run eventually if you then just connect to cellular (although it might take a few minutes).

seadowg · 2024-05-07T16:29:14Z

async/src/main/java/org/odk/collect/async/Scheduler.kt

@@ -81,7 +81,8 @@ interface Scheduler {

    enum class NetworkType {
        WIFI,
-        CELLULAR
+        CELLULAR,
+        OTHER


OTHER would just be any other connected network. Android devices can theoretically have network delivered via Ethernet or Bluetooth for example. These kinds of connections would still satisfy WorkManager's (and our won) "connected" constraint.

seadowg · 2024-05-07T16:30:09Z

async/src/main/java/org/odk/collect/async/TaskSpec.kt

-     * Returns class that can be used to schedule this task using Android's
-     * WorkManager framework
-     */
-    fun getWorkManagerAdapter(): Class<out WorkerAdapter>


I realised we didn't need all this and could just use strings (and no arg constructors) to integrate with WorkManager. It will eventually have to store our class names as strings anyway.

seadowg · 2024-05-07T16:30:45Z

async/src/main/java/org/odk/collect/async/network/ConnectivityProvider.kt

+import android.net.ConnectivityManager
+import org.odk.collect.async.Scheduler
+
+class ConnectivityProvider(private val context: Context) : NetworkStateProvider {


We'll need to replace this implementation at some point, as it's pretty much all deprecated.

seadowg · 2024-05-07T16:34:08Z

collect_app/src/main/java/org/odk/collect/android/instancemanagement/InstancesDataService.kt

@@ -216,6 +214,14 @@ class InstancesDataService(
        }
    }

+    fun instanceFinalized(projectId: String, form: Form) {


I think we'd ideally have a method that's more like:

fun finalizeInstance(projectId: String, instance: Instance)

But the code that finalises a form during form entry and finalises during bulk finalisation is very different at the moment (the former happens pre save to disk), so that's something for another day.

lognaturel · 2024-06-06T14:44:16Z

This one would be really great to get in the next beta if possible. @seadowg did you want @getodk/testers to take another look before review or can @grzesiek2010 take a look?

seadowg · 2024-06-06T14:51:12Z

@lognaturel Yup! Thanks for the nudge. @WKobus are you able to take another look?

..._app/src/androidTest/java/org/odk/collect/android/feature/instancemanagement/AutoSendTest.kt

async/src/main/java/org/odk/collect/async/network/NetworkStateProvider.kt

...src/main/java/org/odk/collect/android/instancemanagement/autosend/InstanceAutoSendFetcher.kt

...test/java/org/odk/collect/android/backgroundwork/FormUpdateAndInstanceSubmitSchedulerTest.kt

...src/main/java/org/odk/collect/android/instancemanagement/autosend/InstanceAutoSendFetcher.kt

grzesiek2010 · 2024-06-06T10:16:40Z

...c/main/java/org/odk/collect/android/backgroundwork/FormUpdateAndInstanceSubmitScheduler.java

        }

        HashMap<String, String> inputData = new HashMap<>();
        inputData.put(TaskData.DATA_PROJECT_ID, projectId);
-        scheduler.networkDeferred(getAutoSendTag(projectId), new AutoSendTaskSpec(), inputData, networkType);
+        scheduler.networkDeferred(getAutoSendTag(projectId), new SendFormsTaskSpec(), inputData, networkConstraint);


I think we don't need to create objects here like new SendFormsTaskSpec(). those objects are never used and we just need the class name to create an object based on it later. That means you can pass class names directly here.

Potentially, I'd need to experiment with TaskSpecWorker using a Kotlin object instead of a class (the way the reflection would work would be different if there's no constructor). Let's look at that when we're next playing with async stuff?

async/src/main/java/org/odk/collect/async/TaskSpecWorker.kt

async/src/test/java/org/odk/collect/async/TaskSpecWorkerTest.kt

async/src/main/java/org/odk/collect/async/TaskSpec.kt

..._app/src/androidTest/java/org/odk/collect/android/feature/instancemanagement/AutoSendTest.kt

grzesiek2010 · 2024-06-06T15:04:32Z

or can @grzesiek2010 take a look?

I was already finishing my review.

…ame changes etc

dbemke · 2024-06-12T09:52:47Z

Tested with Success!

Verified on a device with Android 10

Verified Cases:

issue Forms are not being auto sent after restoring connection that matches auto send setting #6120 doesn’t occur
regression checks in auto send options
finalizing forms and switching to different auto send option that enable/disable sending
hotspot connected and finalizing/sending forms with different auto send options
auto send in different and duplicate projects

WKobus · 2024-06-12T09:54:48Z

Tested with Success!

Verified on a device with Android 14

lognaturel mentioned this pull request May 2, 2024

Auto send should retry if it fails #5946

Closed

1 task

seadowg commented May 7, 2024

View reviewed changes

seadowg marked this pull request as ready for review May 7, 2024 16:55

seadowg requested a review from grzesiek2010 May 7, 2024 16:56

grzesiek2010 requested changes Jun 6, 2024

View reviewed changes

seadowg added 18 commits June 10, 2024 10:59

Isolate deprecated network info code

0e3ae3f

Make TestScheduler network aware to reveal failing autosend tests

dde08ad

Fix form level auto send

f1bca3e

Make sure form level auto send works for multiple forms

8ae1ec2

Simplify auto send logic

77b9edb

Remove extra send method

80a35a5

Use one task spec for sending forms

4cd6530

Support auto send forms in bulk finalization

4889cd7

Rename method

7e90221

Simplify method signature

c862c75

Don't queue multiple form level auto send jobs

e2a81c6

Make sure form level auto send doesn't resend forms

b4d5fde

Simplify TaskSpec and make sure jobs are rescheduled to account for n…

6c8398a

…ame changes etc

Simplify NetworkStateProvider API

4d8c841

Retry cellular tasks if connected to a metered non-cellular connection

1d88a3b

Rework test

8b1f171

Remove test mirroring implementation

8ca2b49

Make sure network state provider is shared properly

57c459c

Set user property when user runs into metered wifi

78a1bda

seadowg force-pushed the form-level-autosend branch from 8f8ef48 to 78a1bda Compare June 10, 2024 10:10

seadowg added 5 commits June 10, 2024 11:10

Remove unneeded line from test

8dd91aa

Fix import

5a90099

Update test names

f08bf48

Remove impossible when clause

52cc9b1

Make test helper private

64355c8

seadowg requested a review from grzesiek2010 June 10, 2024 10:38

grzesiek2010 approved these changes Jun 10, 2024

View reviewed changes

grzesiek2010 added the needs testing label Jun 10, 2024

seadowg merged commit 9833a92 into getodk:master Jun 10, 2024
6 checks passed

seadowg deleted the form-level-autosend branch June 10, 2024 16:26

WKobus added behavior verified and removed needs testing labels Jun 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix autosend scheduling #6121

Fix autosend scheduling #6121

seadowg commented May 2, 2024 •

edited

Loading

seadowg commented May 3, 2024 •

edited

Loading

seadowg May 7, 2024

seadowg May 7, 2024

seadowg May 7, 2024

seadowg May 7, 2024

lognaturel commented Jun 6, 2024

seadowg commented Jun 6, 2024

grzesiek2010 Jun 6, 2024

seadowg Jun 10, 2024

grzesiek2010 Jun 10, 2024

grzesiek2010 commented Jun 6, 2024

dbemke commented Jun 12, 2024

WKobus commented Jun 12, 2024

Fix autosend scheduling #6121

Fix autosend scheduling #6121

Conversation

seadowg commented May 2, 2024 • edited Loading

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Before submitting this PR, please make sure you have:

seadowg commented May 3, 2024 • edited Loading

seadowg May 7, 2024

Choose a reason for hiding this comment

seadowg May 7, 2024

Choose a reason for hiding this comment

seadowg May 7, 2024

Choose a reason for hiding this comment

seadowg May 7, 2024

Choose a reason for hiding this comment

lognaturel commented Jun 6, 2024

seadowg commented Jun 6, 2024

grzesiek2010 Jun 6, 2024

Choose a reason for hiding this comment

seadowg Jun 10, 2024

Choose a reason for hiding this comment

grzesiek2010 Jun 10, 2024

Choose a reason for hiding this comment

grzesiek2010 commented Jun 6, 2024

dbemke commented Jun 12, 2024

WKobus commented Jun 12, 2024

seadowg commented May 2, 2024 •

edited

Loading

seadowg commented May 3, 2024 •

edited

Loading